fix(ci): stabilize visual regression runner and flaky PTY and TUI tests - #14047
Merged
Merged
Conversation
Both visual regression jobs failed in bunx playwright install-deps on blacksmith-4vcpu-ubuntu-2404 because apt could not reach the Ubuntu package servers through the Blacksmith mirror list (run 34574732611). Switch these two jobs to ubuntu-24.04 until Blacksmith confirms a fix.
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (8 files)
Previous Review Summaries (2 snapshots, latest commit 38d4728)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 38d4728)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous review (commit b8a8ae2)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Reviewed by deepseek-v4.1-flash · Input: 0 · Output: 0 · Cached: 0 Review guidance: REVIEW.md from base branch |
The custom answer test pressed keys after a fixed 50 ms sleep, but the provider tree mounts the prompt asynchronously. Under load the options were not on screen yet, the keys hit nothing, and the textarea focus wait timed out on the macOS runner. Render until the options are visible instead.
bun-pty fires onData and onExit once from its read loop and drops events that arrive while no listener is attached. Two races lost them for short lived children such as sh -c 'printf x; exit 7': - The Pty service registers its listeners after yield* Effect.sync(spawn), and the fiber can yield there for 20-100 ms under load. Buffer early events in a latch and replay them when a listener attaches. - The bun-pty Terminal constructor runs the first read synchronously, so output the reader thread queued while the child started fired with no listeners at all. Patch bun-pty to defer the first read by a microtask. Under 6x parallel load the pty tests failed 8 of 24 runs before and pass 36 of 36 with both changes.
marius-kilocode
enabled auto-merge
September 11, 2026 09:26
chrarnoldus
approved these changes
Sep 11, 2026
This was referenced Sep 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Problem This Solves
Three unrelated CI failures kept the pipeline red:
Install Playwright system depsonblacksmith-4vcpu-ubuntu-2404. apt readmirror+file:/etc/apt/blacksmith-ubuntu-mirrors.txtand then could not connect toarchive.ubuntu.comorsecurity.ubuntu.comon port 80, sobunx playwright install-deps chromiumexited with code 100 before any test ran (run 34574732611).unit (macos)timed out inpackages/tui/test/cli/tui/question-custom-answer.test.tsx. The test pressed keys after a fixed 50 ms sleep, but the provider tree mounts the prompt asynchronously, so under load the keys hit nothing and the textarea focus wait expired.unit (macos)timed out inpackages/core/test/pty/pty-session.test.ts(replays exited output and ends when enabled,terminates background descendants, and others) with missing exit events or empty output. This also failed onmain(runs 34509521842 and 34507254245).Why This Change Was Made
Runner. The Playwright deps step cannot be removed; the failing log lists nine packages, including fonts, that are not on the image. Retries do not help a persistent connectivity failure and browser caching does not cover system libraries. The two jobs now run on GitHub-hosted
ubuntu-24.04, which takes a different network path to the Ubuntu package servers. This is marked as temporary in the workflow, with the failing run referenced, so the Blacksmith expression can be restored once the mirror path is confirmed working. The regenerated baselines in this PR come from the new runner.TUI test. The test now renders until the options are on screen before pressing keys. Under 10x parallel load the original failed 2 of 10 runs; the fixed test passed 10 of 10.
PTY. bun-pty fires
onDataandonExitexactly once from its read loop and drops events while no listener is attached. Two races lost them for short-lived children such assh -c 'printf x; exit 7', and also affect real Kilo terminals:Pty.createregisters its listeners afteryield* Effect.sync(spawn). Measured gaps of 20-100 ms at that point under load (the fiber yields viasetImmediate), long enough for the child to exit.packages/core/src/kilocode/pty/latch.tsbuffers early events and replays them once a listener attaches;pty.bun.tswraps the adapter with it.Terminalconstructor runs the firstbun_pty_readsynchronously. If the JS thread is descheduled betweenbun_pty_spawnand that read, the Rust reader thread has already queued the child's output and the first iteration fires with zero listeners. Diagnostics showed the shell alive inwaitwith statusrunningand output"".patches/bun-pty@0.4.8.patchdefers the first read by one microtask; this is a candidate for an upstream bun-pty fix.Under 6x parallel load of the affected pty tests: 8 of 24 runs failed before, 1 of 24 with the latch only, 36 of 36 with both changes. The full pty suites passed 72 of 72 under the same load.
test/kilocode/pty-latch.test.tsreproduces the late-attach case deterministically and fails without the latch.User Impact
Short-lived commands in Kilo terminals (for example Agent Manager scripts) no longer lose their output or fail to report an exit. Everything else is CI only.
Evidence
bun run script/check-workflows.ts: ok (29 workflows)actionlint .github/workflows/visual-regression.yml: passedbun run script/check-opencode-annotations.ts --worktree: passedbun run typecheckinpackages/coreandpackages/tui: passed